Just one question from before that's left over but very happy to approve here! 🙂

does this work for parallel runner? There's some funky stuff going on there with SharedMemoryDataSet so would be good to check it still works.

Thanks for the re-review! You're right I dropped this one somewhere, I'm sorry. I've been looking into this, I'll give an update when I've finished. _SharedMemoryDataset could be a problem.

Thanks for the re-review! You're right I dropped this one somewhere, I'm sorry. I've been looking into this, I'll give an update when I've finished. _SharedMemoryDataset could be a problem.

Cool, no worries. As @noklam says, if it doesn't work then it's not a showstopper. I'm happy to merge with it just working on sequential runner and we can fall back on using the previous inferior _suggest_resume_scenario for the parallel runner case if it's not easy to fix. Would be nice to have it working for parallel runner too, but it's not worth spending a huge amount of time on.

Alright, I finished my investigation into ParallelRunner.

It is possible to implement the new scheme proposed in this PR for ParallelRunner, though it involves some workarounds due to _SharedMemoryDataSet.

Unfortunately, it isn't of much use, since the sequence in which nodes are run (and the resulting exception is reached) is not deterministic for ParallelRunner. This causes problems for both the new and the existing logic for generating suggestions. For example, with the existing logic a run with ParallelRunner will produce the message:

You can resume the pipeline run by adding the following argument to your previous command:
--from-nodes "node3_B,node4_A"

Another identical run will (stochastically) produce the message:

You can resume the pipeline run by adding the following argument to your previous command:
--from-nodes "node4_A"

Similar results are seen for the new logic implemented in this PR. One message is correct while the other isn't. Since these conflicting messages occur with roughly the same frequency, I don't think we should be suggesting a resume command at all at the moment for ParallelRunner. I think implementing this will first require that the order of execution of nodes is made deterministic, which is a large enough task to be a separate PR.

@noklam @AntonyMilneQB it would be good to hear your thoughts on this. If you agree with me, I will turn off this feature for ParallelRunner for the time being in a new commit, merge this PR and then write up an issue.

That sounds like a perfect plan, thanks very much @jmholzer. Note that until recently the sequential runner was also not deterministic in the order of running nodes (something @noklam fixed). I don't know if the same sort of fix would be relevant for the parallel runner.

I am happy that this is added just for SequentialRunner.

Note that there may be 2 sources of non-deterministic behavior:

1. Kedro itself order the nodes in a non-deterministic (Used to be the case with SequentialRunner due to some set operation) -> Distributed these nodes into subprocesses.
2. The nature of parallelism, the order of execution depends on if the computation is finished or not, so I think is non-deterministic by nature.

It's impossible to have deterministic nodes execution order for ParallelRunner, but there may be things that can be more deterministic for 1.

Thanks for the feedback @noklam and @AntonyMilneQB! It's much appreciated.

@noklam thanks for the hint in 1. Regarding 2, you're right about this, the execution order is inherently indeterminate. Nonetheless I think we can at least reach a deterministic 'solution' (in this case, the correct warning) using join(s). I will open an issue and explain my thinking.